Skip to content

Firstresult madness#72

Merged
nicoddemus merged 4 commits into
pytest-dev:masterfrom
goodboy:firstresult_madness
Aug 30, 2017
Merged

Firstresult madness#72
nicoddemus merged 4 commits into
pytest-dev:masterfrom
goodboy:firstresult_madness

Conversation

@goodboy

@goodboy goodboy commented Aug 30, 2017

Copy link
Copy Markdown
Contributor

I think this best solves pytest-dev/pytest#2730 and #71 and is pretty clear.
I'm of course open to a better way if you guys think of one.

Sorry about the delay...

Tyler Goodlet added 2 commits August 30, 2017 14:43
This is to expose the regression from issue pytest-dev#71.
Wrappers must return a single value not a list.
Ensures that wrappers receive a single value instead of a list.

Fixes pytest-dev#71
Comment thread testing/test_hookrelay.py
def hello(self, arg):
return None

class Plugin4(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should add a new test which also changes the outcome result to make sure?

I plan to test this branch in pytest later today as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicoddemus yes good call.
I'll add a force result test.

@nicoddemus

Copy link
Copy Markdown
Member

Sorry about the delay...

Not at all, thanks for tackling this so quickly.

Comment thread pluggy/callers.py
excinfo = sys.exc_info()
finally:
outcome = _Result(results, excinfo)
if firstresult: # first result hooks return a single value

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe it would be more clear to construct the result differently in the firstresult case

if firstresult:
   outcome = _Result(results[0] if results else None, excinfo)
else:
  outcome = _Result(results, excinfo)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks ronny that's way cleaner 👍

Tyler Goodlet added 2 commits August 30, 2017 16:17
Avoids forcing a result with `firstresult` hooks.
Thanks to @RonnyPfannschmidt for the nice simplification.
@goodboy

goodboy commented Aug 30, 2017

Copy link
Copy Markdown
Contributor Author

@RonnyPfannschmidt added your cleaner code.
@nicoddemus added the force result test.

Let me know if we need more.

@nicoddemus

Copy link
Copy Markdown
Member

Tested with pytest and after a few tweaks (__multicall__ issues a warning and pytest is configured to treat warnings as errors) everything seems fine. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants